-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[Sema]: suppress unexpected type warning on some async-let patterns #85615
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
It is somewhat common to use an async-let binding to run a synchronous, void-returning function in an async context. In such cases, the binding's type being inferred as Void is expected, and requiring programmers to explicitly write this to suppress a warning seems like unnecessary boilerplate. This patch changes the diagnostic logic to exempt async-let bindings from the existing diagnostic when the inferred type is Void.
test/decl/var/async_let.swift
Outdated
| // expected-warning @+2 {{constant 'maybeVoid' inferred to have type '()?', which may be unexpected}} | ||
| // expected-note @+1 {{add an explicit type annotation to silence this warning}} | ||
| async let maybeVoid = { Bool.random() ? () : nil }() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if Void? I'm not entirely sure it's worth diagnosing since this can be something that occurs for e.g optional chaining a call to a Void returning method, e.g x?.foo()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you feel like that logic ends at a single level of optionality?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes since optional chaining flattens nested optionals, I don't think a nested optional of Void is likely to be something that comes up very often in practice though, and I'm not sure how much value the warning would have in such cases (it's valuable for regular bindings because the binding itself is probably a mistake, but that's not necessarily the case for async let since the binding does more than provide a value). So I would be fine if we didn't add a special case for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @hamishknight – and just to be sure i'm following correctly: you think it'd be okay to drop the warning for any level of optionally-wrapped Void types on these bindings because 1 level may often be the result of optional chaining, and >1 level is probably uncommon enough to not warrant special handling. does that sound right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes exactly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
follow-up q: do you think the remaining 'unexpected type' checks currently implemented should also apply here, or are there some of those that you think would make sense to suppress as well?
|
@swift-ci please smoke test macos |
|
@swift-ci please smoke test |
It is somewhat common to use an async-let binding to run a synchronous, void-returning function in an async context. In such cases, the binding's type being inferred as Void is expected, and requiring programmers to explicitly write this to suppress a warning seems like unnecessary boilerplate. This patch changes the diagnostic logic to exempt async-let bindings from the existing diagnostic when the inferred type is Void, or an optional thereof.
see: https://forums.swift.org/t/disable-constant-inferred-to-have-type-warning-when-using-async-let/83025